Skip to content

Allow argument-provision checks of templated fields in operator __init__ - #70505

Merged
shahar1 merged 2 commits into
apache:mainfrom
shahar1:validate-init-provision-checks
Jul 28, 2026
Merged

Allow argument-provision checks of templated fields in operator __init__#70505
shahar1 merged 2 commits into
apache:mainfrom
shahar1:validate-init-provision-checks

Conversation

@shahar1

@shahar1 shahar1 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

related: #70296, #70347, #70297

Human Summary

As it turns out, the check I've introduced in #70297 might have been too strict, and we should allow some specific checks to still run in the constructor. This PR relaxes the requirements and removes false positives (according to the new definition) from the exemptions list.

Created a separate issue (#70503) to track reverts.

AI Summary

Click here

validate-operators-init flags every read of a template-field parameter in __init__. That is right for reads of the value, but wrong for reads of provision — "did the author pass this argument?" — which is what mutually-exclusive-argument checks ask.

The constructor is in fact the only place that can answer it. With render_template_as_native_obj=True a provided field renders to None, so the same check in execute() reports a supplied argument as missing. Forcing the move also turns a static authoring mistake into a per-task-instance runtime failure, and — because a rendered value can legitimately be falsy — invites truthiness tests that silently accept conflicting arguments.

This adds one sanctioned pattern: field is None / field is not None on a template-field parameter (or self.<field>) is allowed in __init__. Everything else — truthiness, in, .startswith, isinstance, transformations — stays flagged.

Measured against the burn-down. Replaying both checkers over every exemption entry ever removed (31 PRs, 48 entries): 8 entries across 5 PRs would have needed no operator code change at all — #70341, #70338, #70348, #70392, #70326 moved pure is None exclusivity checks into execute() verbatim. Two cost more than churn: #70348 broke the Rendered Templates view and needed follow-up #70373, and #70326 made a released operator argument required. Five currently-listed entries clear with no code change and are removed here; two of them are already claimed by contributors on #70296.

That replay counted only reverts that are free — entries whose findings drop to zero with no edit to the operator. It says nothing about provision checks written as truthiness (if a and b: raise), which score identically under both checkers and so are invisible to it; those needed a rewrite to stay in __init__, not just the narrowed rule. A separate pass over guard shape found 10 such guards across 6 PRs; the 4 mutually-exclusive ones are tracked on #70503, and the doc now warns that rewriting a required-argument check to is None loosens it.

The remaining 40 entries stay flagged, correctly — they are genuine value reads (ruleset.strip(), isinstance(mongo_query, list), source not in SUPPORTED_SOURCES).

Second half of the change: nested statements are now checked for invalid template-field assignments. Sanctioning the is None read would otherwise let if field is None: self.field = derive(other) through, since the assignment check only looked at top-level statements. A nested assignment still cannot satisfy the requirement that the field be assigned — it may not run — and nested def/class scopes are skipped. That also closes a pre-existing hole with an unrelated guard.

Testing Done
  • scripts/tests/ci/prek/test_validate_operators_init.py: 35 passed. 8 of the 10 new/changed cases fail against the pre-change checker; the 2 that don't are guards against over-sanctioning (truthiness and foo.get('x') is None must stay flagged).
  • Hook run over all 541 files matching its files: regex: exit 0, no stale exemptions.
  • prek run --from-ref main --stage pre-commit: clean (includes mypy for scripts).
Reviewer note: reconciling merged precedent

Five merged burn-down PRs moved pure provision checks into execute() and left comments and regression tests asserting that placement. Nothing breaks — the new rule permits is None in __init__, it does not require it — but those files now demonstrate the pattern the docs discourage, and contributors on #70296 learn the rule by reading them. Reverting them is tracked separately so this PR stays reviewable.

No newsfragment: prek hook, contributing docs, and a clarification to an existing limitations section. Not user-facing behaviour.


Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Opus 5)

Generated-by: Claude Code (Opus 5) following the guidelines

@shahar1
shahar1 requested a review from uranusjr July 27, 2026 08:42
@shahar1 shahar1 added backport-to-v3-3-test Backport to v3-3-test and removed backport-to-v3-3-test Backport to v3-3-test labels Jul 27, 2026
@shahar1
shahar1 force-pushed the validate-init-provision-checks branch from adfa421 to 0026dec Compare July 27, 2026 08:58
mitre88 added a commit to mitre88/airflow that referenced this pull request Jul 27, 2026
The provision checks in GCSDeleteObjectsOperator are false positives
under the relaxed validator definition proposed in apache#70505, so they stay
in the constructor and the class keeps its exemption entry.
@shahar1
shahar1 requested a review from vincbeck July 27, 2026 16:22
The rendering pipeline is one-way with respect to None: an unset field
stays unset, but a field that was passed can render to None. A check that
only asks which arguments the author supplied therefore has to run in the
constructor, and the hook now permits it while still rejecting every read
of the un-rendered value.
@shahar1
shahar1 force-pushed the validate-init-provision-checks branch from 0026dec to b3e2c33 Compare July 27, 2026 21:32
The rule tells contributors to write provision checks as `is None`, but for
a required argument that rewrite starts accepting the empty string the old
truthiness check rejected, silently weakening the operator.
@shahar1
shahar1 merged commit 83968fb into apache:main Jul 28, 2026
34 checks passed
@shahar1
shahar1 deleted the validate-init-provision-checks branch July 28, 2026 06:09
@github-actions

Copy link
Copy Markdown
Contributor

Backport failed to create: v3-3-test. View the failure log Run details

Note: As of Merging PRs targeted for Airflow 3.X
the committer who merges the PR is responsible for backporting the PRs that are bug fixes (generally speaking) to the maintenance branches.

In matter of doubt please ask in #release-management Slack channel.

Status Branch Result
v3-3-test Commit Link

You can attempt to backport this manually by running:

cherry_picker 83968fb v3-3-test

This should apply the commit to the v3-3-test branch and leave the commit in conflict state marking
the files that need manual conflict resolution.

After you have resolved the conflicts, you can continue the backport process by running:

cherry_picker --continue

If you don't have cherry-picker installed, see the installation guide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants